Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MAYA-104987 Allows converting preview surface to Maya native shaders #707

Merged
merged 4 commits into from
Aug 26, 2020

Conversation

JGamache-autodesk
Copy link
Collaborator

@JGamache-autodesk JGamache-autodesk commented Aug 12, 2020

Also allows converting displayColor to other Maya shader nodes.
Also fixes #693 by adding export of Maya standardSurface to displayColors.

Also allows converting displayColor to other Maya shader nodes.
float scale(blinnFn.specularRollOff());
color /= scale;
blinnFn.setSpecularColor(color);
blinnFn.setSpecularRollOff(1.0f);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On export we scale the specular color by the rollOff. So on import we must set the rollOff to 1.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mentioned this in usdMaterialReader.cpp where _OnReadAttribute() gets called, but this code actually looks kind of odd until you realize that this is a "prep" step before we apply the values from USD, since we wouldn't have read a "rolloff" attribute, for example.

Copy link
Contributor

@HamedSabri-adsk HamedSabri-adsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm! Thank you. I did a minimal round trip tests with some of my assets and it is looking good so far. Nice to see the progress in there.

Copy link
Contributor

@mattyjams mattyjams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Just a bunch of mostly cosmetic notes.

@JGamache-autodesk JGamache-autodesk added the ready-for-merge Development process is finished, PR is ready for merge label Aug 26, 2020
@kxl-adsk kxl-adsk merged commit 258aaf2 into dev Aug 26, 2020
@kxl-adsk kxl-adsk deleted the t_gamaj/MAYA-104987/import_converters branch August 26, 2020 14:56
mattyjams added a commit to mattyjams/maya-usd that referenced this pull request Aug 29, 2020
Prior to PR Autodesk#707, the un-linearized transparency value was being used to
compute a transparency "average", which was then used to author displayOpacity
on the bound gprim if it did not already have an authored displayOpacity, as
well as a displayOpacity input on the shader prim. It looks like an unintended
side effect of Autodesk#707 was that it was made to use the linearized transparency for
this purpose instead.

Both of these behaviors seem somewhat dubious. It doesn't really seem like the
shading mode exporter should be authoring displayOpacity of the bound gprim,
since that is more a property of the geometry than the material. It also
doesn't appear as if the displayOpacity input being authored on the shader prim
is used for anything.

Ignoring those two oddities for now, the changes here should address the
"regression" introduced by Autodesk#707. We can revisit the behavior of the
displayColors shading mode (or better yet work towards removing it!) in
follow-up commits.
Comment on lines -117 to +140
const float transparencyAvg = (mayaTransparency[0] +
mayaTransparency[1] +
mayaTransparency[2]) / 3.0f;
const float transparencyAvg = (transparency[0] +
transparency[1] +
transparency[2]) / 3.0f;
Copy link
Contributor

@mattyjams mattyjams Aug 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wanted to flag here that this particular part of the change introduced a subtle "regression" from the old behavior. This is now using the linearized transparency from the lambert shading node rather than the previously un-linearized mayaTransparency.

I opened #751 which restores the previous behavior, but as I note there, this shading mode uses this transparencyAvg value in a somewhat bizarre way. I don't expect it would have any practical impact on anyone exporting materials using the displayColors shading mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
import-export Related to Import and/or Export ready-for-merge Development process is finished, PR is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Standard Surface support for USD export
5 participants